Repository navigation
diagnostics: make a nil respiratory rate say which gate refused it - #1632
Merged
Merged
Conversation
"Respiratory: No data" on a WHOOP 4.0, with 106,673 raw respiratory samples banked and HRV computing on all 22 of the same nights. The log said only `resp day=2026-08-26 rpm=nil`, which is true and useless: finding the cause meant cross-referencing the resp trace against the HRV diagnostic by hand, on a device where 21 of 22 nights were affected. The cause: the RSA estimate needs per-beat-accurate R-R, so beatValuesAreTrustworthy refuses a stream whose intervals are not beat-to-beat measurements. On this strap beatAccurate reads 0.44-0.52 against a 0.50 boundary, with rrIntegrity=crossSecondOverCount — the #1008/#1118 over-count. It straddles the gate, so the card fills on the rare night that clears it and is empty the rest of the time. On the capture that prompted this, the ONE day with a rate (2026-08-11, 16.0 bpm) is the ONE day that measured 0.52; all 22 days match that rule exactly. Print the fraction beside the boundary when the rate is nil. It reports what it knows and no more: below the boundary the gate refused the R-R; at or above it the gate passed and the cause is one of the estimator's four other exits (beats, span, grid, window), which the line deliberately does not guess between. A night that never reached the HRV block emits the original one-field line unchanged. Instrumentation only — no scoring or gating change. Deliberately NOT lowering BEAT_ACCURACY_MIN_FRACTION: a stream at 0.45 is exactly the population the constant's own docstring says we should not guess about, and moving the boundary to fill the card would silently un-withhold SDNN on the same evidence. The fix belongs in #1118. Both platforms, byte-identical output across six cases, diffed rather than eyeballed. 6 JVM tests and a twinned StrandTests file, including the boundary case (0.50 must read as passed, not refused) and the no-HRV-block fallback. Verified: compileFullDebugKotlin clean; full com.noop.analytics suite (1698 tests) green; doc_comment_lint and i18n_audit clean. The Swift edits are app-target and need app-build.
…verge Re-review. Adding a NaN case to the tests exposed a real parity break in the code under test: the JVM renders a non-finite double as "NaN" and C-style %f renders it "nan", so the two platforms would have emitted different bytes for exactly the input the gate treats specially — and the Swift test I had just written would have failed in app-build. Both sides now spell it explicitly rather than leaving it to the formatter. Re-diffed across all seven cases including NaN: byte-identical. The NaN case is worth testing at all because the gate is written as !(f < MIN) deliberately, so NaN lands on "trustworthy" — an unmeasured window must not be silently refused. This line uses the same `<` for the same reason, and a rewrite to `f >= MIN` would read identically for every real number while flipping NaN to "refused". The test pins the mirroring. Also confirmed this pass, both platforms: the gate variables are declared INSIDE the per-day loop (Kotlin `for (offset in 0 until maxDays)` line 754, Swift `for offset in 0..<maxDays` line 869), so one night's figures can never be attributed to the next; and the Swift assignment lands after the complete `var diagLine = … + …` expression rather than inside it. compileFullDebugKotlin clean; 7 JVM tests green; doc_comment_lint and i18n_audit clean.
Re-review, and this one is a correction to my own previous commit. Its `git add` used the main/ path and missed test/, so the Kotlin NaN case stayed in my working tree: the pushed branch carried SIX Kotlin tests against SEVEN Swift ones — a parity break in the PR's own coverage, in a change whose whole subject is keeping the two lines identical. The "7 JVM tests green" line in that commit message described my working tree, not what I had pushed. It is on the branch now. Also corrected a comment two lines above the call site claiming "the Swift diag twin lands with the iOS carry (#1331 follow-up)". It landed some time ago — Swift has respRateLogLine and emits the line — so the comment was telling a reader to expect a gap that is not there, next to code I was already editing. No behaviour change.
ryanbr
added a commit
that referenced
this pull request
Aug 25, 2026
Carries the respiratory nil-reason diagnostic (#1632), so a night whose rate is empty now names the beat-accuracy gate and the fraction that failed it instead of printing a bare nil. Version strings stay 10.6.1; versionCode 363 clears the 362 the last staging build used.
ryanbr
added a commit
that referenced
this pull request
Aug 26, 2026
Attribution alone cannot make a firmware appear. The 5/MG version is read
as four bytes at pay[93] behind a `pay[93] == 50` guard, and the decoder's
own comment says both are anchored to a SINGLE capture at 50.38.1.0: the
name+token region ahead of the version is fixed-width "on this firmware",
re-verifying the offset across firmwares is left as future work, and the
guards deliberately fail closed.
So a 5/MG that does not match reports nothing — and from a log, two very
different causes look identical:
- the byte at 93 is not 50: a different generation marker, offset
probably still right;
- the name+token region is a different width, so byte 93 is NOT the
version and the offset has MOVED. This is the case the decoder comment
warns about, and the one a user cannot diagnose.
Emit the evidence that separates them: payload length, the byte actually at
93, where the printable-ASCII name run ended (the width that shifts
everything after it), and a hex window across the region the version should
occupy. A capture from an undecoded strap then locates the real offset
without another round trip.
The guard is NOT loosened. Widening it blindly would render unrelated bytes
as a version, which is worse than "unknown" because it looks authoritative
— the same reasoning as leaving BEAT_ACCURACY_MIN_FRACTION alone in #1632.
Logged once per connection on both platforms (the value is stable), cleared
on the same per-connection boundary each already uses: Android's reset(),
Swift's `family` didSet.
The decoder half is PURE and lives in WhoopProtocol / Framing, so unlike
most of this cluster it is covered by tests that run on Linux: 5 twinned
cases each, including a truncated payload that must clamp rather than trap,
and a payload ending exactly at the window start.
Verified: 638 WhoopProtocol tests green on Linux; 1031 Android tests green;
the diagnostic diffed Kotlin-vs-Swift across five cases and byte-identical;
doc_comment_lint and i18n_audit clean. The app-side logging is app-target
on Swift and needs app-build.
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The report
"Respiratory: No data" on a WHOOP 4.0 — with 106,673 raw respiratory samples banked and HRV computing on all 22 of the same nights. The strap log said only:
True, and useless. Finding the cause meant cross-referencing the resp trace against the HRV diagnostic by hand.
What it turned out to be
The RSA estimate needs per-beat-accurate R-R, so
beatValuesAreTrustworthyrefuses a stream whose intervals are not beat-to-beat measurements. On this strap:BEAT_ACCURACY_MIN_FRACTIONis 0.50. The device measures 0.44–0.52 — it straddles the boundary, so the card fills on the rare night that clears it and is empty the rest of the time. That is the #1008/#1118 over-count reaching a metric nobody had connected it to.The evidence is exact: the one day with a rate (2026-08-11, 16.0 bpm) is the one day that measured 0.52. All 22 days match that rule, with no exceptions.
The change
Print the fraction beside the boundary when the rate is nil:
It reports what it knows and no more. The estimator has four other NaN exits (too few beats, too short a span, too coarse a grid, bad window); above the boundary the line says the cause is elsewhere rather than naming a gate that did not fire. A night that never reached the HRV block emits the original one-field line, unchanged.
What this deliberately does not do
It does not lower
BEAT_ACCURACY_MIN_FRACTION. A stream at 0.45 is precisely the population the constant's own docstring warns about:Moving the boundary would fill the card by admitting exactly that stream — and would silently un-withhold SDNN on the same evidence. That correction belongs in #1118, where the over-count itself is addressed and beat accuracy returns to ~100% on its own. This PR makes the symptom legible so that fix can be validated.
It is instrumentation only: no scoring, no gating, no stored value changes.
Verification
StrandTestsfile (same seven cases, verified name-for-name). Both include the boundary case — 0.50 must read as passed, not refused, or the line would blame the gate for a night it admitted — and the no-HRV-block fallback.compileFullDebugKotlinclean; fullcom.noop.analyticssuite (1698 tests) green.doc_comment_lintandi18n_audit --ci mainclean.